Skip to content

[Bugfix] Padded Eagle Specdec with Chunked Prefill #26263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Bugfix] Padded Eagle Specdec with Chunked Prefill #26263

wants to merge 2 commits into from
+21 −12

Conversation

Flechman
Copy link
Contributor

@Flechman Flechman commented Oct 5, 2025 •
edited by github-actions bot
Loading

Purpose

Test Plan

Test Result

Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

gemini-code-assist[bot]
gemini-code-assist bot reviewed Oct 5, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request provides a bugfix for padded speculative decoding, specifically within the prepare_next_token_ids_padded function. The change removes a faulty special case for single-token generation (max_gen_len == 1) that did not correctly handle discarded requests. This could have resulted in invalid token IDs being used in subsequent steps. The new implementation generalizes the validity masking logic, making it simpler, more robust, and correct for all scenarios. The fix is sound and improves the correctness of the speculative decoding implementation.

vllm/v1/spec_decode/eagle.py
Comment on lines +525 to +527
valid_mask = (valid_sampled_token_ids_gpu != -1) & (
valid_sampled_token_ids_gpu < gpu_input_batch.vocab_size
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This simplification is a great improvement. By removing the special case for max_gen_len == 1, the code is now more robust. The previous logic didn't account for discarded requests when max_gen_len == 1, which could lead to using an invalid token ID of -1. This unified approach correctly handles all cases.

@Flechman Flechman changed the title [Bugfix] Padded Specdec with Chunked Prefill [Bugfix] Padded Eagle Specdec with Chunked Prefill Oct 6, 2025
@Flechman
Add test
b43e38b 
Signed-off-by: Rémi Delacourt <[email protected]>
@benchislett
Copy link
Collaborator

This bug has been fixed in #26231. I think it would still be nice to merge updated tests, so please update your PR with the fix from main if you wish to continue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benchislett benchislett Awaiting requested review from benchislett benchislett is a code owner

@luccafong luccafong Awaiting requested review from luccafong luccafong is a code owner

1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
speculative-decoding v1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Flechman @benchislett